Skip to content

addPassthru: fix argument order#34245

Merged
orivej merged 1 commit intoNixOS:masterfrom
orivej:addPassthru
Jan 25, 2018
Merged

addPassthru: fix argument order#34245
orivej merged 1 commit intoNixOS:masterfrom
orivej:addPassthru

Conversation

@orivej
Copy link
Contributor

@orivej orivej commented Jan 24, 2018

Motivation for this change

addPassthru became unused in #33057, but its signature was changed at the same time. @chris-martin has informed us at https://groups.google.com/forum/#!topic/nix-devel/YhIRYR7W5wM that this does not make sense.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

addPassthru became unused in NixOS#33057, but its signature was changed at the same
time.  This commit restores the original signature and updates the warning and
the changelog.
@orivej orivej requested a review from vcunat January 24, 2018 23:14
@orivej orivej requested review from edolstra and nbp as code owners January 24, 2018 23:14
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jan 24, 2018
@chris-martin
Copy link
Contributor

Hey, thanks for doing this.

The documentation change is definitely a big plus.

I do think it makes sense to flip the order back to what it was. There's a slim possibility that somebody has fixed a build by updating their calls to the new order, and that we'll just be breaking it for them again by flipping it back. But since the deprecation warning was there, I doubt anyone did this - anyone who was affected by this is probably using extendDerivation now. So, yes, I think this change makes sense, and may spare someone in the future a headache if they are just now switching a build to the latest unstable as I was.

@chris-martin
Copy link
Contributor

chris-martin commented Jan 25, 2018

I'd like to raise another issue, although I don't want it to hold up merging this: The todo comment says that addPassthru should be removed in 18.03. Isn't that premature? Shouldn't the deprecation make it into a release first, and then we remove it in 18.09? Not everyone uses unstable; users of stable releases should have a chance to see the deprecation warning rather than just having the rug pulled out from under them when they upgrade.

@orivej orivej merged commit 3989b33 into NixOS:master Jan 25, 2018
@vcunat
Copy link
Member

vcunat commented Jan 25, 2018

Right. I was focusing too much of the last part of the long-running PR.

I would think that a few months is enough to adapt; it's not a function I'd expect to be used often. When you switch from one stable to another and get a weird error, I'd expect you to look into the incompatible changes section of release notes. (Well, I'd personally do that even before attempting the switch, if it was a production-class system.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants